console,doc: add inspector console object #15579

bengl · 2017-09-23T21:54:36Z

Remove undocumented (and unusable without inspector) methods from
the global console object.

Rather than having the undocumented methods on the global console, these
methods are now available on the console.inspector object.

Fixes: #12675

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

console, doc

BridgeAR · 2017-09-23T22:01:36Z

doc/api/console.md

+
+V8 contexts provide a `console` global object, but by default it's only useful
+for sending console messages to attached inspectors. This is provided as a
+the `inspector` property of the global `console` object (but not of other


The a is to much before the the.

Whoops! Will fix.

TimothyGu · 2017-09-23T22:07:55Z

lib/internal/bootstrap_node.js

-        continue;
-      wrappedConsole[key] = originalConsole[key];
-    }
+    wrappedConsole.inspector = originalConsole;


I would just completely ignore the dummy inspector methods, i.e. do not expose them at all.

If the inspector methods are not provided, users don't have a way of sending any of the messages that aren't also provided by Console.prototype to the inspector console.

I had originally thought of exposing something like require('inspector').console, but I'm not sure that's something I can set up at bootstrap_node.js time, so console.inspector seemed to be a good second option.

@bengl I understand that implication. However, I feel it's ugly from a user experience standpoint to require users to use console.inspector ? console.inspector.table : console.table in polymorphic code: either it should work the way it does everywhere, or it shouldn't.

Also, these console methods only became exposed in 8.0.0, so there isn't much compatibility concern.

For certain functions that inherently do not deal with stdio, like console.markTimeline, console.profile, console.profileEnd, console.timeStamp, console.timeline, and console.timelineEnd, I think we should just keep them as they are and maybe document them. There isn't any work needed by Node.js' Console to "support" those methods because they are no-ops in the console anyway.

That leaves console.debug, console.dirxml, and console.table truly unimplemented Console methods.

@TimothyGu Is that a "-1" opposition or more a "-0"?

refack · 2017-09-24T03:27:09Z

test/parallel/test-console.js

+    assert.notStrictEqual(console[name], console.inspector[name]);
+  }
+
+  // Everything in a v8 console object should be in console.inspector


Aren't you testing the other direction (i.e. that everything that's on console.inspector also exsists on the pristineConsole (i.e. the V8 native)?
So AFAICT the comment should be:

// Everything in `console.inspector` should also be in a v8 pristine console.

refack · 2017-09-24T03:28:48Z

test/parallel/test-console.js

@@ -187,3 +188,15 @@ common.hijackStderr(common.mustCall(function(data) {
    assert.strictEqual(data.includes('no such label'), true);
  });
 }));
+
+const pristineConsole = vm.runInNewContext('this.console');


Maybe you should skip if require('inspector').url() !== undefined?

refack · 2017-09-24T03:29:49Z

test/parallel/test-console.js

+const pristineConsole = vm.runInNewContext('this.console');
+for (const name in console.inspector) {
+
+  // No functions on console that do nothing when no inspector is attached


I'm not sure I understand this comment.

refack · 2017-09-24T03:33:13Z

I like this approach. Only thing is this is "non standard", but I agree that this is the second best thing after require('inspector').console

bengl · 2017-09-24T19:57:48Z

/cc @jasnell since you opened the issue this refs.

jasnell · 2017-09-25T20:46:04Z

doc/api/console.md

@@ -417,6 +417,26 @@ added: v0.1.100

 The `console.warn()` function is an alias for [`console.error()`][].

+### console.inspector
+<!-- YAML
+added: XXXX


nit: should be added: REPLACEME

jasnell · 2017-09-25T20:46:17Z

doc/api/console.md

+added: XXXX
+-->
+
+V8 contexts provide a `console` global object, but by default it's only useful


s/it's/it is

jasnell · 2017-09-25T20:47:14Z

I'm generally good with this approach. I'd like review from other @nodejs/tsc, however. Left a couple of nit comments.

Also, remove undocumented (and unusable without inspector) methods from the global console object. Rather than having the undocumented methods on the global console, these methods are now available on the console.inspector object. Fixes: nodejs#12675

TimothyGu

Making my -1 explicit.

daguej · 2017-11-06T22:32:58Z

Given that these methods are already exposed (if undocumented) on the global console in a LTS release, why not keep and document them? IMO, it makes more sense than moving them to their own nonstandard object (console.inspector?) and breaking things in the process.

BridgeAR · 2017-11-22T18:49:08Z

@bengl @TimothyGu what do you think about using require('inspector').console instead?
Because as it is right now, I do not see it landing and that might be a good alternative?

TimothyGu · 2017-11-22T18:53:54Z

This will not be needed after #17128 is resolved. Since that ticket is pretty active, I recommend closing this PR.

BridgeAR · 2017-11-22T18:56:52Z

Closing due the mentioned alternative.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 23, 2017

bengl added console Issues and PRs related to the console subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 23, 2017

BridgeAR reviewed Sep 23, 2017

View reviewed changes

TimothyGu reviewed Sep 23, 2017

View reviewed changes

refack reviewed Sep 24, 2017

View reviewed changes

bengl force-pushed the bengl/inspectorconsole branch from 0fa0ad1 to d9fef0b Compare September 24, 2017 20:00

jasnell reviewed Sep 25, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 25, 2017

bengl force-pushed the bengl/inspectorconsole branch from d9fef0b to 2a08479 Compare September 27, 2017 02:18

BridgeAR requested a review from a team September 28, 2017 05:55

BridgeAR added the tsc-review label Sep 28, 2017

TimothyGu suggested changes Sep 29, 2017

View reviewed changes

This was referenced Oct 10, 2017

Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-10-11 nodejs/TSC#377

Closed

Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-10-18 nodejs/TSC#384

Closed

daguej mentioned this pull request Nov 6, 2017

Document no-op console methods #16755

Closed

Tiriel mentioned this pull request Nov 14, 2017

change rejected to requested changes nodejs/node-core-utils#109

Merged

BridgeAR closed this Nov 22, 2017

Trott removed the tsc-review label Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console,doc: add inspector console object #15579

console,doc: add inspector console object #15579

bengl commented Sep 23, 2017

BridgeAR Sep 23, 2017

bengl Sep 24, 2017

TimothyGu Sep 23, 2017

bengl Sep 24, 2017

TimothyGu Sep 24, 2017 •

edited

Loading

TimothyGu Sep 24, 2017

Trott Sep 29, 2017

refack Sep 24, 2017

refack Sep 24, 2017

refack Sep 24, 2017

refack commented Sep 24, 2017

bengl commented Sep 24, 2017

jasnell Sep 25, 2017

jasnell Sep 25, 2017

jasnell commented Sep 25, 2017

TimothyGu left a comment

daguej commented Nov 6, 2017

BridgeAR commented Nov 22, 2017

TimothyGu commented Nov 22, 2017

BridgeAR commented Nov 22, 2017

console,doc: add inspector console object #15579

console,doc: add inspector console object #15579

Conversation

bengl commented Sep 23, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 24, 2017

bengl commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 25, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

daguej commented Nov 6, 2017

BridgeAR commented Nov 22, 2017

TimothyGu commented Nov 22, 2017

BridgeAR commented Nov 22, 2017

TimothyGu Sep 24, 2017 •

edited

Loading